-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify feature-handling code #3742
Conversation
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
@@ -74,6 +75,7 @@ mod encode; | |||
pub struct Resolve { | |||
graph: Graph<PackageId>, | |||
replacements: HashMap<PackageId, PackageId>, | |||
empty_features: HashSet<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also possible to make this a function-local lazy-static I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah I prefer this, thanks!
@@ -385,7 +375,7 @@ fn calculate<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) | |||
rustc: util::hash_u64(&cx.config.rustc()?.verbose_version), | |||
target: util::hash_u64(&unit.target), | |||
profile: util::hash_u64(&unit.profile), | |||
features: format!("{:?}", features), | |||
features: format!("{:?}", cx.resolve.features_sorted(unit.pkg.package_id())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the only place where behavior is changed, because we don't print Some
, None
. But fingerprints can change between cargo releases, can't they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah yeah this is totally fine
7c71467
to
2af1c97
Compare
@bors: r+ Awesome cleanup! |
📌 Commit 2af1c97 has been approved by |
⌛ Testing commit 2af1c97 with merge 4716ef8... |
💔 Test failed - status-travis |
2af1c97
to
3267ca5
Compare
Yay, the not rocket science rule has prevented a broken build! Rebased and fixed. |
Quick search showed a handful of instances where we use |
@bors: r+ Yeah I suspect Cargo'd make good use of that |
📌 Commit 3267ca5 has been approved by |
🔒 Merge conflict |
☔ The latest upstream changes (presumably #3733) made this pull request unmergeable. Please resolve the merge conflicts. |
3267ca5
to
50f1c17
Compare
@alexcrichton could you r+ rebased code? :) |
@bors: r+ |
📌 Commit 50f1c17 has been approved by |
Simplify feature-handling code A neat (imo :) ) hack to use an empty `&HashSet` instead of `Option<&HashSet>`.
☀️ Test successful - status-appveyor, status-travis |
A neat (imo :) ) hack to use an empty
&HashSet
instead ofOption<&HashSet>
.